test: fix tests on older IE #3800
Conversation
gkatsev
changed the title
| @@ -139,22 +139,22 @@ const TestHelpers = { | |||
| * @param {string} eventType | |||
| */ | |||
| triggerDomEvent(element, eventType) { | |||
| let event; | |||
There was a problem hiding this comment.
this change is for removing windows line-endings.
There was a problem hiding this comment.
good catch! (windows newline removals)
| }); | ||
|
|
||
| QUnit.test('should not allow to register custom player when any player has been created', function(assert) { | ||
| const tag = TestHelpers.makeTag(); | ||
| const player = videojs(tag); | ||
|
|
||
| class CustomPlayer extends Player {} | ||
| try { | ||
|
|
||
| assert.throws(function() { |
There was a problem hiding this comment.
assert.throws is better.
| @@ -1269,19 +1280,23 @@ QUnit.test('should allow to register custom player when any player has not been | |||
|
|
|||
| assert.equal(player instanceof CustomPlayer, true, 'player is custom'); | |||
| player.dispose(); | |||
|
|
|||
| // reset the Player to the original value; | |||
| videojs.registerComponent('Player', Player); | |||
There was a problem hiding this comment.
This fixes the actual issue with custom player on IE8.
| if (Player.players && Object.keys(Player.players).length > 0) { | ||
| if (Player.players && | ||
| Object.keys(Player.players).length > 0 && | ||
| Object.keys(Player.players).map((playerName) => Player.players[playerName]).every(Boolean)) { |
There was a problem hiding this comment.
Not sure whether we want a comment here but basically, if all the items in Player.players are null, you're allowed to register a custom player. This is partly so we can reset the Player component properly for the test.
There was a problem hiding this comment.
I think it deserves some kind of comment. Basically, what you just wrote! 😆
| if (Player.players && Object.keys(Player.players).length > 0) { | ||
| if (Player.players && | ||
| Object.keys(Player.players).length > 0 && | ||
| Object.keys(Player.players).map((playerName) => Player.players[playerName]).every(Boolean)) { |
There was a problem hiding this comment.
I think it deserves some kind of comment. Basically, what you just wrote! 😆
| @@ -139,22 +139,22 @@ const TestHelpers = { | |||
| * @param {string} eventType | |||
| */ | |||
| triggerDomEvent(element, eventType) { | |||
| let event; | |||
There was a problem hiding this comment.
good catch! (windows newline removals)
| @@ -435,7 +436,11 @@ test('chapters should be displayed when remote track added and load event fired' | |||
|
|
|||
| equal(chaptersEl.track.cues.length, 2); | |||
|
|
|||
| TestHelpers.triggerDomEvent(chaptersEl, 'load'); | |||
| if (player.tech_.featuresNativeTextTracks) { | |||
| TestHelpers.triggerDomEvent(chaptersEl, 'load'); | |||
There was a problem hiding this comment.
why/how are these two statements different?
There was a problem hiding this comment.
On IE8 we use a custom DOM element for text tracks, however, IE8 doesn't allow us to trigger custom events on a custom element.
So, because everywhere we support native text tracks, we can trigger a real DOM event, we use that and where we use emulated tracks, we just trigger load directly on the chapters element object.
There was a problem hiding this comment.
I can add a comment in the code for this, if you think it's necessary.
There was a problem hiding this comment.
So should this check for IE8 instead? or maybe just add a comment to specify that this is a protection for IE8?
There was a problem hiding this comment.
I decided that a general solution would be better than specifically checking for IE8. However, I'll add a comment about this.
No description provided.